From BlenderWiki
Code Review
--Brecht 13:20, 14 July 2011 (CEST)
Coding conventions
-
There are some mixed tabs/spaces used, those should all become tabs.-> done -
Use $Id$ in the file header to have svn automatically set the filename, last modified date, etc put there (now it's just $Id without the end $).-> done -
Also #includes are usually organized per module (prefix), with the local includes last.-> done
Generate Seam operator
- Recursion depth 1 is not a good default, I guess this option will get automated somehow with the stretch metric?
--- Stretch metric is already used. The recursion depth is just the maximum depth the user want to go. If the stretch become to grow before reaching the max depth, the algorithm is automatically terminated that time.
-
Calculate Combinatorial is not a user friendly name, if this is an option that is just temporary that's fine, otherwise need to find a better name & description for what this does.- done. - There are some printf() error messages, some of those should be communicated to the user using BKE_report(op->reports, ..) like other operators.
--- To do that, we have to pass the operator to the autoseam module which is in blender/intern. I am not sure will it be a good task to pass the reference of an operator to blender/intern module.
-
Face selection is not used, it just generates the seam for the entire mesh. Edit mode operators should take this into acount and never modify unselected elements.-> done. Seams are only generated on selected faces now. If no face is selected, a warning is showed to the user.
- I think generate same should only do the unwrap afterwards if Live Unwrap is enabled, for consistency, but I'm not sure.
autoseam_tools.c
- I would move more code into the autoseam module, and have callbacks to mark seams and compute stretch. It feels like the algorithm straddles these two module now, I would rather see the autoseam module API smaller with fewer functions.
-
All functions in this file except MESH_OT_generate_seam can become static.-> done. -
There doesn't seem to be a good reason to have a separate .h file here, and blender conventions is to avoid putting #includes in .h files, I'd just merge autoseam_tools.h in this file.- done
ED_uvedit.h
-
minimize_stretch_* doesn't need to be here anymore I guess.-> done.
Autoseam module
- Module convention is to put external header files into a separate directory of the internal implementation.
- Files here are missing the license header.
- I'd like to see some comments about what each class does in the header files, to give an idea of how the pieces fit together.
Overall
- It's good to see the tool working, but I haven't seen evidence yet that this code works better than e.g. "smart project". Testing on more complex models once the sparse solver is added may reveal there's still tweaks and improvements needed for this to become a useful tool.
- So plan for that, don't assume it will work well enough once all bullet points are done, but start testing with real models as soon as possible.
- Regarding next two weeks planning, is it really that much work to get arpack/lanczos working? If you replace dense Eigen matrices by sparse ones, then you should really only need a wrapper function or two to call the library.
